-
Notifications
You must be signed in to change notification settings - Fork 18
Move charts to stimulus #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaces legacy Highcharts modules with Stimulus controllers and helpers: removes Changes
Sequence DiagramssequenceDiagram
actor User
participant View as View (shots show/compare)
participant Controller as ShotChartsController
participant Highcharts as Highcharts
participant DOM as Browser DOM
User->>View: Load shot/compare page
View->>Controller: connect (data-*-value: shot, stages, temperature, comparison)
Controller->>Controller: parse data, create/destroy charts as needed
Controller->>Highcharts: initialize shot chart
Controller->>Highcharts: initialize temperature chart
Highcharts->>DOM: render charts into targets
User->>DOM: mouse move / hover
DOM->>Controller: forward event
Controller->>Controller: find hover point, sync crosshairs/tooltips
Controller->>Highcharts: update tooltips/crosshairs across charts
User->>DOM: toggle annotations / change compare input
DOM->>Controller: trigger handlers
Controller->>Controller: update annotations / apply offset
Controller->>Highcharts: update series/plotlines
sequenceDiagram
actor User
participant View as View (stats/index)
participant Controller as StatsChartsController
participant Highcharts as Highcharts
participant DOM as Browser DOM
User->>View: Load stats page
View->>Controller: connect (data-*-value: uploadedChartData, userChartData)
Controller->>Controller: initialize charts array
alt uploadedChartData present
Controller->>Highcharts: create uploaded chart
end
alt userChartData present
Controller->>Highcharts: create user chart
end
Highcharts->>DOM: render charts
User->>View: navigate away
View->>Controller: disconnect
Controller->>Highcharts: destroy charts
Controller->>Controller: clear references
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/javascript/controllers/stats_charts_controller.js`:
- Around line 16-19: The forEach callback in disconnect() implicitly returns
chart.destroy(), triggering the Biome lint rule; update the callback so it does
not return a value—either convert the arrow to a block body that calls
chart.destroy(); (i.e. use { chart.destroy(); }) or replace the forEach with an
explicit loop (for...of over this.charts) to call destroy on each chart, then
reset this.charts = [].
In `@app/javascript/helpers/object_helpers.js`:
- Around line 1-13: The deepMerge function exits early because the for...in loop
uses "return" when encountering inherited properties; change that "return" to
"continue" so the loop skips inherited keys without returning the whole
function. Also replace the unsafe obj2.hasOwnProperty(key) check with a safer
form such as Object.hasOwn(obj2, key) or
Object.prototype.hasOwnProperty.call(obj2, key) to avoid issues when obj2
shadows hasOwnProperty; keep the rest of deepMerge (result, recursion when
obj2[key] and obj1[key] are objects) unchanged.
🧹 Nitpick comments (1)
app/javascript/controllers/shot_charts_controller.js (1)
150-164: Consider scoping the annotation button to avoid conflicts with multiple chart instances.Using
document.getElementById("remove-annotations")creates a global ID that could conflict if multiple shot chart controllers exist on the same page. Consider using a Stimulus target or scoping the query to the controller's element.♻️ Suggested approach using scoped query
- chart.renderer.text('<button id="remove-annotations" class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</span>', 50, 35, true).attr({ zIndex: 3 }).add() + chart.renderer.text('<button class="remove-annotations inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</span>', 50, 35, true).attr({ zIndex: 3 }).add() chart.annotationVisible = true - document.getElementById("remove-annotations").addEventListener("click", function () { + this.element.querySelector(".remove-annotations").addEventListener("click", function () {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/javascript/controllers/shot_charts_controller.js`:
- Around line 124-167: The button markup in setupInCupAnnotations is invalid (it
closes with </span>) and uses a static id "remove-annotations" which will
collide when multiple charts are on the page; fix by rendering a proper <button>
element (correct closing tag) via the chart.renderer call (or
renderer.createElement) and keep the returned element reference (e.g., save the
value returned from chart.renderer.text(...).add() or the created element)
instead of using document.getElementById; attach the click event to that element
reference, toggle annotation visibility using
chart.controller.destroyShotStages()/drawShotStages() and update the element's
label (use textContent) so each chart has its own independent control.
- Around line 29-48: The shotStagesNormalized mapping is assigning id: x (the
whole stage object) which breaks Highcharts' addPlotLine/removePlotLine that
expect a scalar id; change the mapping in initializeCharts (where
shotStagesNormalized is set) to use a stable scalar like id: x.value (or
String(x.value)) instead of the full object, and ensure any usage in
drawShotStages (and any code that calls
chart.xAxis[0].addPlotLine/removePlotLine) references that scalar id.
| setupInCupAnnotations(chart) { | ||
| const weightColor = chart.series.find(x => x.name === "Weight Flow").color | ||
| const timings = this.shotStagesNormalized.map(x => x.value) | ||
| const weightFlow = this.extractStages("Weight Flow", timings) | ||
| const weight = this.extractStages("Weight", timings) | ||
|
|
||
| let labels = [] | ||
| timings.forEach((timing, index) => { | ||
| const inCup = weight[index] | ||
| if (inCup > 0) { | ||
| labels.push({ | ||
| text: `${inCup}g`, | ||
| point: { x: timing, y: weightFlow[index], xAxis: 0, yAxis: 0 }, | ||
| y: -30, | ||
| x: -30, | ||
| allowOverlap: true, | ||
| style: { color: weightColor }, | ||
| borderColor: weightColor | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| chart.inCupAnnotation = chart.addAnnotation({ | ||
| draggable: false, | ||
| labels: labels, | ||
| labelOptions: { shape: "connector" } | ||
| }) | ||
|
|
||
| chart.renderer.text('<button id="remove-annotations" class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</span>', 50, 35, true).attr({ zIndex: 3 }).add() | ||
| chart.annotationVisible = true | ||
| document.getElementById("remove-annotations").addEventListener("click", function () { | ||
| if (chart.annotationVisible) { | ||
| chart.controller.destroyShotStages() | ||
| chart.annotations[0].graphic.hide() | ||
| chart.annotationVisible = false | ||
| this.innerHTML = "Show annotations" | ||
| } else { | ||
| chart.controller.drawShotStages() | ||
| chart.annotations[0].graphic.show() | ||
| chart.annotationVisible = true | ||
| this.innerHTML = "Hide annotations" | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n app/javascript/controllers/shot_charts_controller.js | sed -n '124,167p'Repository: miharekar/visualizer
Length of output: 2160
🏁 Script executed:
# Search for instances of shot_charts_controller usage
rg -t js "shot_charts_controller" --max-count 10
# Check if there are multiple charts rendered on same page
rg -t html -t erb "shot_charts_controller" --max-count 20Repository: miharekar/visualizer
Length of output: 46
🏁 Script executed:
# Search for similar patterns in the codebase using Highcharts renderer
rg "renderer\.text\(" -A 2 -B 2Repository: miharekar/visualizer
Length of output: 853
🏁 Script executed:
# Search for Stimulus controller binding in views
rg -t erb -t html "shot-charts|shot_charts" -l
# Also check for any data-controller attributes
rg "data-controller" -t erb -t html | head -20Repository: miharekar/visualizer
Length of output: 3213
🏁 Script executed:
# Check Highcharts documentation or similar patterns - look at method chaining
rg "\.add\(\)" -B 5 -A 5 app/javascript/controllers/shot_charts_controller.jsRepository: miharekar/visualizer
Length of output: 803
🏁 Script executed:
# Check the full shot_charts_controller.js to understand renderer API usage
cat -n app/javascript/controllers/shot_charts_controller.js | head -50Repository: miharekar/visualizer
Length of output: 2189
🏁 Script executed:
# Search for other uses of .add() or event listeners on renderer elements
rg "\.add\(\)" app/javascript/controllers/shot_charts_controller.js -A 3 -B 3Repository: miharekar/visualizer
Length of output: 668
🏁 Script executed:
# Check for other event listeners in the controller
rg "addEventListener|\.on\(" app/javascript/controllers/shot_charts_controller.jsRepository: miharekar/visualizer
Length of output: 471
🏁 Script executed:
# Look at the full context around the problematic code
cat -n app/javascript/controllers/shot_charts_controller.js | sed -n '150,180p'Repository: miharekar/visualizer
Length of output: 1686
Fix invalid button markup and avoid ID collisions on multi-chart pages.
Line 152 closes the <button> with </span>, and the static id="remove-annotations" causes collisions when multiple charts exist on the same page (e.g., the compare view). Use the renderer element directly with a direct reference instead of querying by ID.
🐛 Suggested fix
- chart.renderer.text('<button id="remove-annotations" class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</span>', 50, 35, true).attr({ zIndex: 3 }).add()
+ const button = chart.renderer
+ .text('<button class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</button>', 50, 35, true)
+ .attr({ zIndex: 3 })
+ .add()
chart.annotationVisible = true
- document.getElementById("remove-annotations").addEventListener("click", function () {
+ button.element.addEventListener("click", function () {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setupInCupAnnotations(chart) { | |
| const weightColor = chart.series.find(x => x.name === "Weight Flow").color | |
| const timings = this.shotStagesNormalized.map(x => x.value) | |
| const weightFlow = this.extractStages("Weight Flow", timings) | |
| const weight = this.extractStages("Weight", timings) | |
| let labels = [] | |
| timings.forEach((timing, index) => { | |
| const inCup = weight[index] | |
| if (inCup > 0) { | |
| labels.push({ | |
| text: `${inCup}g`, | |
| point: { x: timing, y: weightFlow[index], xAxis: 0, yAxis: 0 }, | |
| y: -30, | |
| x: -30, | |
| allowOverlap: true, | |
| style: { color: weightColor }, | |
| borderColor: weightColor | |
| }) | |
| } | |
| }) | |
| chart.inCupAnnotation = chart.addAnnotation({ | |
| draggable: false, | |
| labels: labels, | |
| labelOptions: { shape: "connector" } | |
| }) | |
| chart.renderer.text('<button id="remove-annotations" class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</span>', 50, 35, true).attr({ zIndex: 3 }).add() | |
| chart.annotationVisible = true | |
| document.getElementById("remove-annotations").addEventListener("click", function () { | |
| if (chart.annotationVisible) { | |
| chart.controller.destroyShotStages() | |
| chart.annotations[0].graphic.hide() | |
| chart.annotationVisible = false | |
| this.innerHTML = "Show annotations" | |
| } else { | |
| chart.controller.drawShotStages() | |
| chart.annotations[0].graphic.show() | |
| chart.annotationVisible = true | |
| this.innerHTML = "Hide annotations" | |
| } | |
| }) | |
| } | |
| setupInCupAnnotations(chart) { | |
| const weightColor = chart.series.find(x => x.name === "Weight Flow").color | |
| const timings = this.shotStagesNormalized.map(x => x.value) | |
| const weightFlow = this.extractStages("Weight Flow", timings) | |
| const weight = this.extractStages("Weight", timings) | |
| let labels = [] | |
| timings.forEach((timing, index) => { | |
| const inCup = weight[index] | |
| if (inCup > 0) { | |
| labels.push({ | |
| text: `${inCup}g`, | |
| point: { x: timing, y: weightFlow[index], xAxis: 0, yAxis: 0 }, | |
| y: -30, | |
| x: -30, | |
| allowOverlap: true, | |
| style: { color: weightColor }, | |
| borderColor: weightColor | |
| }) | |
| } | |
| }) | |
| chart.inCupAnnotation = chart.addAnnotation({ | |
| draggable: false, | |
| labels: labels, | |
| labelOptions: { shape: "connector" } | |
| }) | |
| const button = chart.renderer | |
| .text('<button class="inline-flex py-1 px-2 text-xs font-medium bg-white rounded border shadow-sm cursor-pointer highcharts-no-tooltip border-neutral-300 text-neutral-700 dark:border-neutral-600 dark:bg-neutral-800 dark:text-neutral-300 dark:hover:bg-neutral-900 hover:bg-neutral-50">Hide annotations</button>', 50, 35, true) | |
| .attr({ zIndex: 3 }) | |
| .add() | |
| chart.annotationVisible = true | |
| button.element.addEventListener("click", function () { | |
| if (chart.annotationVisible) { | |
| chart.controller.destroyShotStages() | |
| chart.annotations[0].graphic.hide() | |
| chart.annotationVisible = false | |
| this.innerHTML = "Show annotations" | |
| } else { | |
| chart.controller.drawShotStages() | |
| chart.annotations[0].graphic.show() | |
| chart.annotationVisible = true | |
| this.innerHTML = "Hide annotations" | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@app/javascript/controllers/shot_charts_controller.js` around lines 124 - 167,
The button markup in setupInCupAnnotations is invalid (it closes with </span>)
and uses a static id "remove-annotations" which will collide when multiple
charts are on the page; fix by rendering a proper <button> element (correct
closing tag) via the chart.renderer call (or renderer.createElement) and keep
the returned element reference (e.g., save the value returned from
chart.renderer.text(...).add() or the created element) instead of using
document.getElementById; attach the click event to that element reference,
toggle annotation visibility using
chart.controller.destroyShotStages()/drawShotStages() and update the element's
label (use textContent) so each chart has its own independent control.
Use a stable plotline id and render the toggle button with valid HTML so it displays and updates correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/javascript/controllers/shot_charts_controller.js`:
- Around line 221-225: In drawShotStages(), the inner forEach uses an arrow
expression that implicitly returns the result of addPlotLine (violates the lint
rule); change the callbacks to block bodies that call addPlotLine without
returning values — e.g., in drawShotStages replace
this.shotStagesNormalized.forEach(x => chart.xAxis[0].addPlotLine(x)) with
this.shotStagesNormalized.forEach(x => { chart.xAxis[0].addPlotLine(x); }) (and
similarly ensure the outer charts.forEach uses a block body if currently an
implicit return).
- Around line 152-175: The annotation toggle button created in
setupInCupAnnotations (chart.annotationButton / toggleButton) is left behind
when updateInCupVisibility removes annotations, causing duplicate buttons and
crashes; modify the code path in updateInCupVisibility to remove the button and
its listener when annotations are removed (e.g., if chart.annotationButton
exists call its destruction/removal and clear chart.annotationButton and
chart.annotationVisible), and harden the toggleButton click handler in
setupInCupAnnotations to guard before accessing chart.annotations[0] (check
chart.annotations && chart.annotations[0] && chart.annotations[0].graphic) so
the listener no-ops if annotations are missing. Ensure any added event listeners
are removed (or use a single attached handler) when the button is removed.
Clean up annotation toggle handlers when annotations are removed and avoid implicit returns in plotline rendering.
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.